WIP: Possible alternate implementation of UseConsistentWhitespace CheckOperator feature#1607
Draft
daviesj wants to merge 3 commits intoPowerShell:mainfrom
Draft
WIP: Possible alternate implementation of UseConsistentWhitespace CheckOperator feature#1607daviesj wants to merge 3 commits intoPowerShell:mainfrom
daviesj wants to merge 3 commits intoPowerShell:mainfrom
Conversation
bergmeister
reviewed
Oct 30, 2020
Collaborator
There was a problem hiding this comment.
Thanks for the detailed explanation. Happy to take that and looks ok to me. I see it's largely based on #1566, therefore we should aim to get that PR merged soon. @rjmholt Can you please give your final yes/no on that other PR as you've already reviewed it before? I myself am happy with the other PR already.
After that the diff in this PR will be much smaller but as you said I can already see that the main diff will be just the change of using your Ast visitor instead of TokenTraits.HasTrait.
This PR will likely cause a merge conflict for my PR #1602 but I am happy to accept that.
| private IEnumerable<DiagnosticRecord> FindOperatorViolations(TokenOperations tokenOperations) | ||
| { | ||
| foreach (var tokenNode in tokenOperations.GetTokenNodes(IsOperator)) | ||
| foreach (LinkedListNode<Token> tokenNode in tokenOperations.GetTokenNodes((t)=>true)) |
Collaborator
There was a problem hiding this comment.
Suggested change
| foreach (LinkedListNode<Token> tokenNode in tokenOperations.GetTokenNodes((t)=>true)) | |
| foreach (LinkedListNode<Token> tokenNode in tokenOperations.GetTokenNodes(()=>true)) |
Contributor
|
Converting this to a draft while it remains a WIP |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
OK, so this far from ready, but I wanted to get it out there as a suggestion. It all started with trying to work on unary operator issues describe in #1239. But after discovering the problem in #1606, I was thinking maybe the best thing would be to re-implement the feature and an idea of how to do it came to me.
I started by writing a bunch more tests (3a47f4a) both as a way to establish a baseline for how the feature behaves now, and to hopefully be a tool to help with any fixes for the linked issues. In order to come up with this list of tests, I looked through about_Operators and related topics. I added a test for any type of operator that I felt was somehow different, but didn't already have a test and could possibly fall into the category of operators that one might possibly want
CheckOperatorto enforce whitespace around (or to ignore).Then, I wrote a new implementation (d3da1e4) which more-or-less corresponds with the current behavior, except (at least in my opinion) handles unary operators better and fixes the inconsistencies with bitwise operators between PS 5.1 and 7. Since it already passed the tests for most types of operators I decided to go ahead and write code that might handle the other types (3943377). I realize that that code may not be complete enough and maybe not all of those operators should be implemented.
Hopefully the code comments explain the strategy pretty well but I will add a couple things. The code for the visitor is the same as I used in PR #1566. Also, I chose to remove the
IsOperatorfunction because it wasn't used anywhere else, and the tests that would be used just to determine whether the token is an operator are so similar to the ones used to determine what sort of operator, I figured it would be simpler not to run through them twice.With this implementation, it would also be reasonably easy to enforce no whitespace around operators like
++,--,-,!as described in #1239 (which could also easily be made optional). See possible implementation.PR Checklist
.cs,.ps1and.psm1files have the correct copyright headerWIP:to the beginning of the title and remove the prefix when the PR is ready.